Basic vdo tests#10
Conversation
Add mounted_fs() for filesystem test setup with automatic cleanup, and the SLAB_BITS_SMALL constant. Assisted-by: Claude Opus 4.6 <claude-opus-4-6>
Converts Basic01.pm, BasicFSDedupe.pm, Create03.pm, and UDSTimeout01.pm to Python. basic_fs_dedupe_tests.py (from BasicFSDedupe.pm): - Disables lazy_itable_init on ext4 (via new fs.py enhancement) to prevent background metadata writes from skewing dedup stats - Tolerance widened for statistics verification: 0.1/0.05 vs Perl's 0.01 uds_timeout_tests.py (from UDSTimeout01.pm): - Perl uses configurable 100ms VDO timeout; Python relies on upstream dm_vdo's fixed 5000ms timeout exceeded by 6000ms dm-delay read delay - Dataset size reduced: 10 x 20,000 blocks (200K total) vs Perl's 10 x 150,000 (1.5M total) - Two-phase approach: populate UDS index on fast storage first, then restart VDO on dm-delay device - Uses dm-delay table-swap technique to avoid udev probe hang - Disables delay after 60 seconds to let remaining I/O drain quickly Infrastructure: - fs.py: Added lazy_itable_init parameter for ext4 formatting Assisted-by: Claude Opus 4.6 <claude-opus-4-6>
lorelei-sakai
left a comment
lorelei-sakai
left a comment
There was a problem hiding this comment.
Okay, first note: 4 new tests is a reasonable size for a PR. We could prbaly even do a few more, especially if they're thematically related to each other or share code.
And the tests themselves are mostly fine. (I noted a few places where I could not see the reasoning behind a difference from the perl originals.)
Most comment are about structuring gthe utilities and trying to make a standardize format and tools for all our tests going forward. So if we get those taken care of, hat should make the subsequent PRs simpler to review too; we shouldn't need to relitigate those questions.
I do want to confirm that you've run these tests against the current main branch of dmtest-python, though. There are few things that make me suspect these tests don't even run right now, much less pass.
Also I note that you aren't adding any tags. Fine for this PR, but we really ought to get a tagging strategy set and in use before the default vdo test set gets too large.
| raise AssertionError("VDO not online within 30 seconds") | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
This is a lot of blank lines. I was expecting maybe two, this is like 6?
| @contextmanager | ||
| def mounted_fs(dev, fs_class=None, format=False, **format_opts): | ||
|
|
||
| Yields the mount point path and ensures unmount, fsck, and mount |
There was a problem hiding this comment.
This "Yields" comment looks like it should be a docstring, but no quotes?
Did you actually run tests that use this?
| Yields the mount point path and ensures unmount, fsck, and mount | ||
| point removal on exit. | ||
| if fs_class is None: | ||
| fs_class = Ext4 |
There was a problem hiding this comment.
Why pick Est4 as the default? I'm not sure it's bad, but I think our default for perl tests is XFS now (mirroring RHEL's default, I think).
| if format: | ||
| fs.format(**format_opts) | ||
| with tempfile.TemporaryDirectory() as mount_point: | ||
| fs.mount(mount_point) |
There was a problem hiding this comment.
BaseFS already has a mount_and_chdir(), is there a particular reason not to use that here?
|
|
||
| def t_basic(fix): | ||
| """Basic VDO functional test: write files, stop/start VDO, verify data persists.""" | ||
| # Create VDO with slab_bits=17 (SLAB_BITS_SMALL) |
There was a problem hiding this comment.
This commenting style is going to get real old real fast. This comment doesn't say anything that the very next line of code doesn't also say. Well, other than the name of the SLAB_BITS_SMALL constant, which you could fix by actually using the new constant in the test.
And more importantly, this is true about basically every comment in this test. Possibly the stop are restart comments at lines 4 2 and 47 are useful.
Please remove the comments that don't provide extra information.
| data_dev, 0, 0, data_dev, 0, 0) | ||
| ) | ||
| delay_dev = dmdev.dev(zero_delay_table) | ||
| try: |
There was a problem hiding this comment.
I'm not 100% positive, but sine you're just swapping the table, not tearing the device down, I think you can use the standard "with device" spelling for the delay device also. Probably cleaner than writing your own try/finally.
| vdo_path, BLOCK_COUNT, BLOCK_SIZE, second_offset | ||
| ) | ||
| br.write(tag=tag) | ||
| except Exception as e: |
There was a problem hiding this comment.
Can these writes error out? And if they can, can we make it so they don't error out?
| vdo_path, BLOCK_COUNT, BLOCK_SIZE, second_offset | ||
| ) | ||
| br.write(tag=tag) | ||
| except Exception as e: |
There was a problem hiding this comment.
In my recovery testing, an error in a thread would fail the whole test anyway; do we need this error aggregation? But I suppose it doesn't hurt anything at any rate, and it's not too complicated. Probably fine.
| after_timeouts = after_stats['dedupeAdviceTimeouts'] | ||
| log.info(f"Dedupe advice timeouts after phase 2: {after_timeouts}") | ||
|
|
||
| assert after_timeouts > before_timeouts, ( |
There was a problem hiding this comment.
Looks barbaric to not have parens or some delimiter around the condition, but I guess that's just how python is? Also I wish we had a better condition, but I concede it matches the original test.
|
|
||
| BLOCK_COUNT = 20000 | ||
| DATASET_COUNT = 10 | ||
| READ_DELAY_MS = 6000 |
There was a problem hiding this comment.
I might be worth a comment here that this delay is longer that the UDS timeout (5 seconds), which I assume is intentional. But we're comparing a hard-coded number here to a hard-coded number in the driver, which we could easily break in the future if we're not careful.
2 participants
No description provided.